Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add debug logging support #1903

Merged
merged 23 commits into from
Mar 4, 2025
Merged

feat: add debug logging support #1903

merged 23 commits into from
Mar 4, 2025

Conversation

feywind
Copy link
Contributor

@feywind feywind commented Dec 19, 2024

Adds debug logging support for auth.

It's also waiting on this to finish:
googleapis/gax-nodejs#1669

This is a duplicate of another PR to make the GitHub bot macguffins go.

@feywind feywind requested review from a team as code owners December 19, 2024 21:31
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Dec 19, 2024
@feywind feywind added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2024
@feywind feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 20, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 20, 2024
@d-goog
Copy link
Collaborator

d-goog commented Feb 21, 2025

Hello! With recent enhancements in the library we may be able to greatly streamline this PR by adding a listener here (thanks to the removal of Transporter):

  • static readonly DEFAULT_REQUEST_INTERCEPTOR: Parameters<
    Gaxios['interceptors']['request']['add']
    >[0] = {
    resolved: async config => {
    // Set `x-goog-api-client`, if not already set
    if (!config.headers.has('x-goog-api-client')) {
    const nodeVersion = process.version.replace(/^v/, '');
    config.headers.set('x-goog-api-client', `gl-node/${nodeVersion}`);
    }
    // Set `User-Agent`
    const userAgent = config.headers.get('User-Agent');
    if (!userAgent) {
    config.headers.set('User-Agent', USER_AGENT);
    } else if (!userAgent.includes(`${PRODUCT_NAME}/`)) {
    config.headers.set('User-Agent', `${userAgent} ${USER_AGENT}`);
    }
    return config;
    },
    };

Every auth request goes through this function. Let me know if you’d like to sync on this or have any questions.

@feywind
Copy link
Contributor Author

feywind commented Feb 21, 2025

@d-goog Ah, lovely. Thanks!

@@ -107,8 +109,10 @@ export class UserRefreshClient extends OAuth2Client {
refresh_token: this._refreshToken,
target_audience: targetAudience,
} as {}),
});
};
AuthClient.setMethodName(opts, 'fetchIdToken');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely optional: Since these classes extend AuthClient we could use the class's setMethodName and avoid importing AuthClient.

Suggested change
AuthClient.setMethodName(opts, 'fetchIdToken');
UserRefreshClient.setMethodName(opts, 'fetchIdToken');

Here and in other places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, after thinking about it, I'm not a super fan of static inheritance like that. I think it makes it more obscure what's actually being called. (Also, several of the other usages were not deriving from anything that had setMethodName so the usage was mixed.) I'm okay with it either way, but maybe leaning toward not changing it.

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Mar 4, 2025
@feywind feywind merged commit 13ca1dc into main Mar 4, 2025
17 checks passed
@feywind feywind deleted the add-debug-logging branch March 4, 2025 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants